Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: dynamically load releasers in ./releasers directory #439

Merged
merged 5 commits into from
May 19, 2020

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented May 19, 2020

This refactor pulls some additional logic to the ReleasePR class (it now has releaserName which is used rather than an enum for looking up a specific release type, and lookupPackageName method, which was a feature being done in an ad hoc way for only Node.js).

BREAKING CHANGE: this removers the ReleaseType enum, in favor of a dynamically generated list of loaders.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2020
@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #439 into master will decrease coverage by 9.99%.
The diff coverage is 84.21%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #439       +/-   ##
===========================================
- Coverage   89.51%   79.51%   -10.00%     
===========================================
  Files          32       40        +8     
  Lines        3643     4487      +844     
  Branches      314      334       +20     
===========================================
+ Hits         3261     3568      +307     
- Misses        381      918      +537     
  Partials        1        1               
Impacted Files Coverage Δ
src/updaters/php-client-version.ts 88.09% <ø> (-0.28%) ⬇️
src/github.ts 75.29% <60.00%> (+0.10%) ⬆️
src/release-pr-factory.ts 72.72% <60.00%> (ø)
src/release-pr.ts 89.28% <81.81%> (-0.92%) ⬇️
src/releasers/index.ts 86.36% <86.36%> (ø)
src/releasers/node.ts 35.20% <87.50%> (ø)
src/github-release.ts 94.55% <88.23%> (-0.90%) ⬇️
src/graphql-to-commits.ts 89.56% <100.00%> (ø)
src/releasers/java-auth-yoshi.ts 24.28% <100.00%> (ø)
src/releasers/java-bom.ts 93.62% <100.00%> (+0.02%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a270337...136a4ea. Read the comment docs.

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please add the new enum value for simple.

@bcoe bcoe changed the title feat: attempt to lookup package name in well known location refactor!: dynamically load releasers in ./releasers directory May 19, 2020
@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2020

export function getReleaserNames(): string[] {
return releasers.map(releaser => {
console.info(releaser.releaserName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need this logging?

!file.name.match(/index\.js/)
) {
const obj = require(`./${file.name}`) as {[key: string]: typeof ReleasePR};
releasers.push(obj[Object.keys(obj)[0]]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, I would probably add them to an object instead, so that we could get one by accessing it by name directly and without a for loop. But I'm OK either way.

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 19, 2020
@bcoe bcoe merged commit e3b13a9 into master May 19, 2020
@release-please release-please bot mentioned this pull request May 19, 2020
@chingor13 chingor13 deleted the lookup-package-name branch September 27, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants